Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: verdi data trajectory show #5394

Merged
merged 30 commits into from
Jul 11, 2023
Merged

fix: verdi data trajectory show #5394

merged 30 commits into from
Jul 11, 2023

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Feb 25, 2022 •
edited
Loading

fix #2435

The verdi data trajectory show command was broken at least since
dc7cdd0 (Jul 2018).

The fact that most of its formats rely on external programs makes
it somewhat tedious to test.

@ltalirz
Copy link
Member Author

ltalirz commented Feb 25, 2022 •
edited
Loading

two formats fixed (jmol, xcrysden, at least superficially; did not look at the output yet)
one more to go (won't be installing mayavi for the last one)

@ltalirz
Copy link
Member Author

ltalirz commented Feb 25, 2022 •
edited
Loading

@ryotatomioka Since I don't really have any real-world trajectorydata at hand, would you be able to export a sample trajectory via verdi archive create -N 12345 -- archive.aiida and attach it to this PR?

This would allow me to check whether, now that the code is supposedly working, the output of verdi data trajectory show actually looks useful.

@sphuber
Copy link
Contributor

sphuber commented May 17, 2023

@ltalirz is this PR still relevant? What still needs to be done for this to be merged or closed?

@danielhollas
Copy link
Collaborator

@ltalirz @sphuber LMK if you still need some sample TrajectoryData to finish this, I can provide those. (and since I make a heavy use of TrajectoryData, I am interresting in this PR getting merged).

@ltalirz
Copy link
Member Author

ltalirz commented May 18, 2023

@danielhollas if you can test out this branch that would be very welcome.

I currently don't have time to finish it, please feel free to take it from here

@sphuber sphuber force-pushed the issue-2435-trajectory-show branch from fac2af9 to 5b603a0 Compare July 6, 2023 23:44
@ltalirz
Copy link
Member Author

ltalirz commented Jul 7, 2023

thanks for taking this on; feel free to merge this directly - if you want any further input let me know

@sphuber sphuber marked this pull request as ready for review July 7, 2023 01:11
ltalirz and others added 6 commits July 7, 2023 04:00
@ltalirz @sphuber
fix: verdi data trajectory show
00d0b20 
The `verdi data trajectory show` command was broken at least since
dc7cdd0 (Jul 2018).

The fact that most of its formats rely on external programs makes
it somewhat tedious to test.
@sphuber
Fix the tests
612ad0d 
The tests were failing because `mayavi` is not installed and so was
raising an `ImportError`. In addition, the tests would hang because the
show command would actually open a window to display the generated image
for certain formats. Since the purpose of these CLI tests are not
actually to test the implementation of the visualization but just the
CLI command, the show function is mocked with a function that simply
checks that the expected arguments are passed, based on the options that
are passed to the command line invocation.
@sphuber
Increase test coverage
9f548cb
@sphuber
Test
3888ab8
@sphuber
skip all
1bcfa4a
@sphuber
Testing
1e4971f
@sphuber sphuber force-pushed the issue-2435-trajectory-show branch from c78636c to aebe1fb Compare July 7, 2023 16:02
@sphuber sphuber force-pushed the issue-2435-trajectory-show branch 2 times, most recently from 2a3826b to 1bcfa4a Compare July 8, 2023 04:33
@sphuber sphuber force-pushed the issue-2435-trajectory-show branch 5 times, most recently from ae8d13e to cf811fe Compare July 10, 2023 18:19
@sphuber
Copy link
Contributor

sphuber commented Jul 10, 2023

After an arduous battle with a legion of ghosts in the machine, I declare victory and denounce matplotlib as being the culprit! This commit proves that merely importing matplotlib.pyplot in the verdi data core.trajectory show command causes a completely unrelated test of the Manager.unload_profile method to fail. The latter checks that after unloading a profile (which should call StorageBackend.close() and which should relinquish all resources of that storage), the sqlalchemy.orm.session._sessions weak ref dictionary no longer holds any references. For some unknown reason, when importing matplotlib in the trajectory test, the storage test fails because there still is a ref in the dictionary. It is completely unclear what the connection is but at this point I no longer care 😅

sphuber
sphuber approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I am calling it on this one 😄 thanks @ltalirz !

@sphuber sphuber merged commit fd4c126 into aiidateam:main Jul 11, 2023
@sphuber sphuber deleted the issue-2435-trajectory-show branch July 11, 2023 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryotatomioka ryotatomioka ryotatomioka left review comments

@sphuber sphuber sphuber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

verdi data trajectory show broken
4 participants
@ltalirz @sphuber @danielhollas @ryotatomioka